API: Handle exceptions from ApiBeforeMain hook in a user-friendly manner

The immediate impetus behind this change is this series of events:

1. CirrusSearch hooks ApiBeforeMain to handle some setup that requires
   the User object.
2. So User is loaded from the session.
3. OAuth checks the headers as part of loading User.
4. OAuth sees that the headers are invalid, and since it was called from
   the API it throws a UsageException, expecting the API to catch it and
   return an appropriate response to the client.
5. But nothing does so, leading to an unhelpful "Internal Error" page
   being returned to the client.

We can do better than that.

Bug: 62312
Change-Id: Ib5735661eec6ebe57eaa69c67b399e703cc90fc4

diff --git a/api.php b/api.php
index 0d2312a..554c272 100644 (file)
--- a/api.php
+++ b/api.php
@@ -70,10 +70,21 @@ $wgTitle = Title::makeTitle( NS_MAIN, 'API' );
 $processor = new ApiMain( RequestContext::getMain(), $wgEnableWriteAPI );
 
 // Last chance hook before executing the API
-wfRunHooks( 'ApiBeforeMain', array( &$processor ) );
+try {
+       wfRunHooks( 'ApiBeforeMain', array( &$processor ) );
+       if ( !$processor instanceof ApiMain ) {
+               throw new MWException( 'ApiBeforMain hook set $processor to a non-ApiMain class' );
+       }
+} catch ( Exception $e ) {
+       // Crap. Try to report the exception in API format to be friendly to clients.
+       ApiMain::handleApiBeforeMainException( $e );
+       $processor = false;
+}
 
 // Process data & print results
-$processor->execute();
+if ( $processor ) {
+       $processor->execute();
+}
 
 if ( function_exists( 'fastcgi_finish_request' ) ) {
        fastcgi_finish_request();
@@ -97,11 +108,15 @@ if ( $wgAPIRequestLog ) {
                $_SERVER['HTTP_USER_AGENT']
        );
        $items[] = $wgRequest->wasPosted() ? 'POST' : 'GET';
-       $module = $processor->getModule();
-       if ( $module->mustBePosted() ) {
-               $items[] = "action=" . $wgRequest->getVal( 'action' );
+       if ( $processor ) {
+               $module = $processor->getModule();
+               if ( $module->mustBePosted() ) {
+                       $items[] = "action=" . $wgRequest->getVal( 'action' );
+               } else {
+                       $items[] = wfArrayToCgi( $wgRequest->getValues() );
+               }
        } else {
-               $items[] = wfArrayToCgi( $wgRequest->getValues() );
+               $items[] = "failed in ApiBeforeMain";
        }
        wfErrorLog( implode( ',', $items ) . "\n", $wgAPIRequestLog );
        wfDebug( "Logged API request to $wgAPIRequestLog\n" );

diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index eb24a35..8f270dc 100644 (file)
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -363,37 +363,7 @@ class ApiMain extends ApiBase {
                try {
                        $this->executeAction();
                } catch ( Exception $e ) {
-                       // Allow extra cleanup and logging
-                       wfRunHooks( 'ApiMain::onException', array( $this, $e ) );
-
-                       // Log it
-                       if ( !( $e instanceof UsageException ) ) {
-                               MWExceptionHandler::logException( $e );
-                       }
-
-                       // Handle any kind of exception by outputting properly formatted error message.
-                       // If this fails, an unhandled exception should be thrown so that global error
-                       // handler will process and log it.
-
-                       $errCode = $this->substituteResultWithError( $e );
-
-                       // Error results should not be cached
-                       $this->setCacheMode( 'private' );
-
-                       $response = $this->getRequest()->response();
-                       $headerStr = 'MediaWiki-API-Error: ' . $errCode;
-                       if ( $e->getCode() === 0 ) {
-                               $response->header( $headerStr );
-                       } else {
-                               $response->header( $headerStr, true, $e->getCode() );
-                       }
-
-                       // Reset and print just the error message
-                       ob_clean();
-
-                       // If the error occurred during printing, do a printer->profileOut()
-                       $this->mPrinter->safeProfileOut();
-                       $this->printResult( true );
+                       $this->handleException( $e );
                }
 
                // Log the request whether or not there was an error
@@ -410,6 +380,73 @@ class ApiMain extends ApiBase {
                ob_end_flush();
        }
 
+       /**
+        * Handle an exception as an API response
+        *
+        * @since 1.23
+        * @param Exception $e
+        */
+       protected function handleException( Exception $e ) {
+               // Allow extra cleanup and logging
+               wfRunHooks( 'ApiMain::onException', array( $this, $e ) );
+
+               // Log it
+               if ( !( $e instanceof UsageException ) ) {
+                       MWExceptionHandler::logException( $e );
+               }
+
+               // Handle any kind of exception by outputting properly formatted error message.
+               // If this fails, an unhandled exception should be thrown so that global error
+               // handler will process and log it.
+
+               $errCode = $this->substituteResultWithError( $e );
+
+               // Error results should not be cached
+               $this->setCacheMode( 'private' );
+
+               $response = $this->getRequest()->response();
+               $headerStr = 'MediaWiki-API-Error: ' . $errCode;
+               if ( $e->getCode() === 0 ) {
+                       $response->header( $headerStr );
+               } else {
+                       $response->header( $headerStr, true, $e->getCode() );
+               }
+
+               // Reset and print just the error message
+               ob_clean();
+
+               // If the error occurred during printing, do a printer->profileOut()
+               $this->mPrinter->safeProfileOut();
+               $this->printResult( true );
+       }
+
+       /**
+        * Handle an exception from the ApiBeforeMain hook.
+        *
+        * This tries to print the exception as an API response, to be more
+        * friendly to clients. If it fails, it will rethrow the exception.
+        *
+        * @since 1.23
+        * @param Exception $e
+        */
+       public static function handleApiBeforeMainException( Exception $e ) {
+               ob_start();
+
+               try {
+                       $main = new self( RequestContext::getMain(), false );
+                       $main->handleException( $e );
+               } catch ( Exception $e2 ) {
+                       // Nope, even that didn't work. Punt.
+                       throw $e;
+               }
+
+               // Log the request and reset cache headers
+               $main->logRequest( 0 );
+               $main->sendCacheHeaders();
+
+               ob_end_flush();
+       }
+
        /**
         * Check the &origin= query parameter against the Origin: HTTP header and respond appropriately.
         *